Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds an exists? class method #343

Merged
merged 9 commits into from
Jul 3, 2019
Merged

Conversation

Blacksmoke16
Copy link
Contributor

@Blacksmoke16 Blacksmoke16 commented Jun 22, 2019

Adds a method that will return true if a record exists that matches the provided criteria, or false otherwise. Based off of ActiveRecord.exists?.

  • If given a String or Number, attempts to lookup a records with a PK with that value.
  • If given a Hash or NamedTuple, attempts to lookup a record that matches the given criteria.
  • Refactors some of the querying logic to support find_by(Hash), and to share some code between exists? and find_by
  • Fixes a bug with find_by that made it not correctly match nil values.
  • Fixes the type restriction on various querying methods to allow Array and UUID values.

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@drujensen
Copy link
Member

@amberframework/contributors Any other feedback? If not, I will merge.

@robacarp
Copy link
Member

Hm, I think I'd expect the syntax to be Model.where(col: val).exists?.

The type based overloading makes me a little nervous, but I think it's well thought out.

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jun 27, 2019

@robacarp That's also an option. What would we do if they did like Model.where(col: val).order(:created_at).limit(50) Just ignore the order and limit?

It should be all tested as well. Helped clean everything up a bit.

@robacarp
Copy link
Member

@Blacksmoke16 man it'd be great if it weren't possible by compile-time-enforcement to call #exists? on a query which had #limit called on it. However, I think that's a bigger refactor than is needed here, and that just raising a runtime warning of some sort is fine.

To be clear, I don't think having a Class#exists? method is mutually exclusive with a Class#where()#exists?, I think we can provide both.

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jun 28, 2019

To be clear, I don't think having a Class#exists? method is mutually exclusive with a Class#where()#exists?, I think we can provide both.

Right, as long as we're OK with just ignoring other parts of the query I can implement this. Im imaging it to just be a new assembler method like select but does the exists query instead.

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jun 30, 2019

@robacarp I added that if you wanted to review again.

Going to see if the MySQL issue will be resolved soon, otherwise I'll monkey patch it and can remove the patch when it does.

Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one comment.

src/granite/query/assemblers/base.cr Show resolved Hide resolved
src/granite/query/builder.cr Show resolved Hide resolved
src/adapter/pg.cr Show resolved Hide resolved
src/granite/query/executors/value.cr Show resolved Hide resolved
src/granite/querying.cr Show resolved Hide resolved
Refactor the build_find_by_clause method
@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jul 1, 2019

@robacarp that's updated. I also set the MySQL shard to the latest commit so that CI passes, as crystal-lang/crystal-mysql#78 got merged. Can be changed back to the version string next time a new release is made.

@robacarp
Copy link
Member

robacarp commented Jul 2, 2019

@drujensen thoughts on targeting a non-release version of mysql? A commit hash is just as unique as a version number but since it's pinned to the exact commit could it cause version conflicts with projects which require mysql themselves?

@Blacksmoke16 any idea when that'll be released to a numbered version?

@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Jul 2, 2019

For us it's only a development dependency. But yea, someone else using the mysql shard would have to use master or that commit for this feature to work.

I do not, that would be a question for @bcardiff.-~

It's in PR to be released. I changed it back to use the version string. Will rerun CI when it is released.

@drujensen
Copy link
Member

@robacarp looks like 0.7.0 will be released soon crystal-lang/crystal-mysql#79

@Blacksmoke16
Copy link
Contributor Author

Should be good to go now @drujensen @robacarp

@robacarp robacarp merged commit 6f2e6a3 into amberframework:master Jul 3, 2019
@Blacksmoke16 Blacksmoke16 deleted the exists branch July 3, 2019 21:31
@robacarp
Copy link
Member

robacarp commented Jul 3, 2019

thanks as always @Blacksmoke16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants